fix(security): harden HTTP redirects by trust boundary#2634
Conversation
fl0rianr
left a comment
There was a problem hiding this comment.
Thanks for addressing this. I agree that external downloads should be restricted to HTTPS and that redirect chains should be bounded. However, the current change applies the HTTPS-only policy to the shared HttpClient::get() implementation, which is also used for communication with wrapped backends on http://127.0.0.1:<port>.
This is already causing real regressions rather than CI-only failures:
- Test .exe - llamacpp`fails with CURL error: Unsupported protocol
- Test .exe - audio-gen-acestep fails for the same reason
I do not think CI should be converted to HTTPS here. Production wrapped backends intentionally use loopback HTTP, so CI should continue testing that path.
I suggest separating the policies by trust boundary:
- External downloads / external requests: allow only https, follow redirects, restrict redirect protocols to https, and set
MAXREDIRS. - Lemonade-managed loopback backends: allow only
httpand disable redirects entirely. - User-configured HTTP mirrors, if supported: require an explicit opt-in such as
allow_insecure_http, disabled by default.
This could be implemented with a small HttpRequestOptions or HttpPolicy parameter and a shared helper that applies the corresponding curl options. The loopback callers in WrappedServer and ACE-Step should then explicitly select the trusted-loopback policy.
A few additional points:
-
Restricting protocols is useful hardening, but it does not fully prevent SSRF. HTTPS URLs can still target loopback/private addresses or resolve to them through DNS. The PR description should either narrow the claim to protocol and downgrade protection, or add resolved-address and redirect-target validation.
-
Lemonade requires libcurl >= 8.5.0, where redirects are already bounded by default.
MAXREDIRS=5is good defense in depth, but the description should not claim the existing behavior allows infinite redirect loops. -
Please check the return values of the security-related
curl_easy_setopt()calls so failure to apply a restriction fails closed. -
Please add focused tests for:
- trusted loopback HTTP GET without redirects,
- rejection of initial HTTP under the external policy,
- rejection of HTTPS-to-HTTP or non-HTTP(S) redirects,
- the configured redirect limit.
So I support the intended hardening, but the current global HTTPS-only change should not be merged as-is.
|
Thanks for the great rework. Two minor follow-ups: |
4f6195a to
cc11556
Compare
cc11556 to
27be686
Compare
fl0rianr
left a comment
There was a problem hiding this comment.
Thanks for rebasing and getting CI back on track. The original trust-boundary refactor still looks good, the title is now accurate, and the current workflows are green.
However, the latest loopback-provider fix introduces a new blocking issue.
The new is_loopback() logic selects TrustedLoopback based only on the parsed host. This also classifies a URL such as https://localhost:PORT/v1 as trusted loopback, but TrustedLoopback permits only the http protocol. A valid local HTTPS provider would therefore fail with Unsupported protocol.
The manual authority parsing is also not safe enough for a security-boundary decision:
const auto colon = host.find(':');
if (colon != std::string::npos) host = host.substr(0, colon);This does not correctly handle bracketed IPv6 and can confuse URL userinfo with the hostname. For example, http://localhost:80@evil.example/... is parsed here as localhost, while the actual destination host is evil.example.
I suggest removing this automatic loopback exception entirely:
- HTTPS provider URLs should use ExternalHttpsOnly.
- HTTP provider URLs should use AllowInsecureHttp only when
allow_insecure_http=true. - TrustedLoopback should remain limited to Lemonade-generated backend URLs whose destination is already known to be 127.0.0.1.
Local cloud proxies and their tests should explicitly enable the existing insecure-HTTP opt-in. That keeps the implementation aligned with the PR description and avoids a second ad-hoc URL parser.
There is also a problem with the new HTTPS-to-HTTP downgrade test. It connects with https:// to a plain httplib::Server, so the TLS handshake fails before the /http-redirect handler can return a redirect. The test then accepts any non-CURLE_OK result, meaning it passes on an unrelated TLS or connection failure and does not verify CURLOPT_REDIR_PROTOCOLS_STR.
Please either remove that test or run a real TLS test server and verify that:
- the HTTPS route was reached,
- it returned the redirect,
- the HTTP target was not reached, and
- curl returned the expected redirect-protocol error.
The main policy implementation is otherwise in good shape, but I would fix these two points before approval.
866b612 to
8ae5da6
Compare
Fixes SWSPLAT-24200 by adding security restrictions to all CURLOPT_FOLLOWLOCATION usage in HttpClient to prevent SSRF, protocol downgrade attacks, and redirect-loop DoS. Changes: - Add CURLOPT_MAXREDIRS=5L to limit redirect chains - Add CURLOPT_PROTOCOLS_STR="https" to restrict initial protocol - Add CURLOPT_REDIR_PROTOCOLS_STR="https" to restrict redirect protocols This prevents: - HTTPS→HTTP downgrade attacks during backend binary downloads - FTP/FILE protocol SSRF to internal network endpoints - Infinite redirect loops causing DoS Applied to all three locations that use CURLOPT_FOLLOWLOCATION: 1. HttpClient::get() - line 375 2. HttpClient::download_attempt() - line 683 3. HttpClient::download_attempt() HEAD request - line 842 Co-Authored-By: Claude <noreply@anthropic.com>
Fixes SWSPLAT-24200 by splitting HttpClient security restrictions by trust boundary so loopback backend RPC keeps working over http. Policies (HttpSecurityPolicy): - External hosts (HF, GitHub, release CDNs): https-only for the initial request and every redirect hop, MAXREDIRS=5. - Lemonade-managed loopback backends: http only, redirects disabled entirely. - Opt-in plaintext endpoints (cloud allow_insecure_http): http+https, bounded redirects. A shared helper (apply_http_security_policy) applies the curl options and checks each setopt return value, so a restriction that fails to apply fails closed. Added test for HTTPS-to-HTTP protocol downgrade rejection under the external policy.
…_models refresh_cloud_models checked allow_insecure_http before proceeding but then called discover_models() without passing it, so discovery fell back to the ExternalHttpsOnly policy and rejected loopback/opted-in http:// providers with "Unsupported protocol". Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
8ae5da6 to
5f4e167
Compare
fl0rianr
left a comment
There was a problem hiding this comment.
Thanks for the continued rework. I reviewed the current head again from the trust-boundary selection through the call sites, download path, tests, and CI state.
The two issues from my previous review have been addressed correctly:
- the ad-hoc loopback URL parser has been removed,
- the false-positive HTTPS test has been removed,
- and
refresh_cloud_models()now forwards the configuredallow_insecure_httpvalue.
The core HttpSecurityPolicy design also looks good: external GETs and downloads default to HTTPS-only, managed backend loopback requests use HTTP without redirects, and failures to apply the security-relevant curl options fail closed.
There is still one blocking policy-selection issue in CloudServer::discover_models().
The current selection is based only on the boolean:
allow_insecure_http
? HttpSecurityPolicy::AllowInsecureHttp
: HttpSecurityPolicy::ExternalHttpsOnlyThis means an HTTPS provider with allow_insecure_http=true also receives AllowInsecureHttp, which permits redirects to HTTP. The install API and config format allow the flag to remain true for an HTTPS URL, so this is not merely an unreachable state.
That weakens an HTTPS provider unnecessarily and contradicts the PR's stated boundary that the opt-in applies to plaintext endpoints. It is especially important here because the discovery request carries an Authorization: Bearer ... header.
Please derive the policy from both the scheme and the opt-in:
const auto policy =
allow_insecure_http &&
CloudProviderRegistry::is_http_base_url(normalized_base)
? utils::HttpSecurityPolicy::AllowInsecureHttp
: utils::HttpSecurityPolicy::ExternalHttpsOnly;This keeps HTTPS providers HTTPS-only even if the flag is stale or accidentally set, while explicitly opted-in HTTP providers continue to work.
Please also add a focused regression test for the policy decision:
- HTTPS + allow_insecure_http=false → ExternalHttpsOnly
- HTTPS + allow_insecure_http=true → ExternalHttpsOnly
- HTTP + allow_insecure_http=true → AllowInsecureHttp
One non-blocking observation: a protocol rejection in download_file() is currently treated like a general non-resumable transfer failure, so the outer loop can retry it repeatedly and remove an existing partial file. Treating CURLE_UNSUPPORTED_PROTOCOL and malformed URLs as immediate permanent failures would avoid unnecessary retries and preserve partial data, but this does not need to block the present PR.
Once the HTTPS-policy selection is corrected and the remaining CI workflows complete successfully, I would be comfortable approving the PR.
An HTTPS provider with a stale allow_insecure_http=true was selecting AllowInsecureHttp, which permits redirect downgrades to http on the Bearer-carrying discovery request. Derive the policy from both the URL scheme and the opt-in so HTTPS providers stay HTTPS-only, extracted into CloudServer::discovery_policy() with focused regression tests. Also treat CURLE_UNSUPPORTED_PROTOCOL and CURLE_URL_MALFORMAT as permanent download failures so the retry loop stops immediately and preserves any existing partial file. Co-Authored-By: Claude <noreply@anthropic.com>
fl0rianr
left a comment
There was a problem hiding this comment.
Thanks for the hard iterating! Finally you have my approval.
One non-blocking nit: after a permanent failure exits on the first attempt, the final error string still reports max_retries + 1 attempts. That can be cleaned up separately and does not need to hold this PR.
…2634) * fix(security): restrict libcurl redirect protocols to HTTPS only Fixes SWSPLAT-24200 by adding security restrictions to all CURLOPT_FOLLOWLOCATION usage in HttpClient to prevent SSRF, protocol downgrade attacks, and redirect-loop DoS. Changes: - Add CURLOPT_MAXREDIRS=5L to limit redirect chains - Add CURLOPT_PROTOCOLS_STR="https" to restrict initial protocol - Add CURLOPT_REDIR_PROTOCOLS_STR="https" to restrict redirect protocols This prevents: - HTTPS→HTTP downgrade attacks during backend binary downloads - FTP/FILE protocol SSRF to internal network endpoints - Infinite redirect loops causing DoS Applied to all three locations that use CURLOPT_FOLLOWLOCATION: 1. HttpClient::get() - line 375 2. HttpClient::download_attempt() - line 683 3. HttpClient::download_attempt() HEAD request - line 842 Co-Authored-By: Claude <noreply@anthropic.com> * fix(security): harden HTTP redirects by trust boundary Fixes SWSPLAT-24200 by splitting HttpClient security restrictions by trust boundary so loopback backend RPC keeps working over http. Policies (HttpSecurityPolicy): - External hosts (HF, GitHub, release CDNs): https-only for the initial request and every redirect hop, MAXREDIRS=5. - Lemonade-managed loopback backends: http only, redirects disabled entirely. - Opt-in plaintext endpoints (cloud allow_insecure_http): http+https, bounded redirects. A shared helper (apply_http_security_policy) applies the curl options and checks each setopt return value, so a restriction that fails to apply fails closed. Added test for HTTPS-to-HTTP protocol downgrade rejection under the external policy. * fix(cloud): use correct security policy for loopback provider discovery * fix(cloud): forward allow_insecure_http to discovery in refresh_cloud_models refresh_cloud_models checked allow_insecure_http before proceeding but then called discover_models() without passing it, so discovery fell back to the ExternalHttpsOnly policy and rejected loopback/opted-in http:// providers with "Unsupported protocol". Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * fix(cloud): derive discovery policy from scheme and opt-in An HTTPS provider with a stale allow_insecure_http=true was selecting AllowInsecureHttp, which permits redirect downgrades to http on the Bearer-carrying discovery request. Derive the policy from both the URL scheme and the opt-in so HTTPS providers stay HTTPS-only, extracted into CloudServer::discovery_policy() with focused regression tests. Also treat CURLE_UNSUPPORTED_PROTOCOL and CURLE_URL_MALFORMAT as permanent download failures so the retry loop stops immediately and preserves any existing partial file. Co-Authored-By: Claude <noreply@anthropic.com> --------- Co-authored-by: Claude <noreply@anthropic.com>
Fixes SWSPLAT-24200 by applying scheme and redirect hardening to
HttpClient's redirect-following requests, split by trust boundary so loopback backend RPC keeps working over http.Policies (
HttpSecurityPolicy):MAXREDIRS=5.allow_insecure_http): http+https, bounded redirects.A shared helper applies the curl options and checks each
setoptreturn value, so a restriction that fails to apply fails closed.Scope of protection:
httpsURL can still resolve to loopback/private addresses; resolved-address validation is out of scope here.MAXREDIRS=5is defense-in-depth: libcurl already bounds redirects by default at our >= 8.5.0 floor, so the prior behavior did not permit infinite redirect loops.Tests:
test/cpp/test_http_client_security.cppcovers trusted-loopback GET without redirects, rejection of initial http under the external policy, the configured redirect limit, and rejection of non-http(s) redirects.